New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat:added format flag #8080

Merged

knative-prow merged 4 commits into knative:main from EraKin575:delivery-format

Jul 9, 2024

Contributor

EraKin575 commented Jul 5, 2024 •

edited

Loading

Fixes #8073
added format flag to deliverytype struct to support more destination of cloudevents

Proposed Changes

Pre-review Checklist

At least 80% unit test coverage
E2E tests for any new behavior
Docs PR for any user-facing impact
Spec PR for any new API feature
Conformance test for any change to the spec

Release Note

Docs


          added format flag

4fe8fda

Signed-off-by: EraKin575 <tejaskumar574@gmail.com>

knative-prow bot requested review from aliok and lionelvillard

July 5, 2024 05:55

knative-prow bot added the size/S label

knative-prow bot commented Jul 5, 2024

Welcome @EraKin575! It looks like this is your first PR to knative/eventing 🎉

knative-prow bot commented Jul 5, 2024

Hi @EraKin575. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

knative-prow bot added the needs-ok-to-test label


          corrected lint

Signed-off-by: EraKin575 <tejaskumar574@gmail.com>

pierDipi reviewed

View reviewed changes

Member

pierDipi left a comment

Thanks @EraKin575, left a few comments

pkg/apis/duck/v1/delivery_types.go Outdated

@@ @@ -81,6 +81,11 @@ type DeliverySpec struct { @@
               	//
               	// +optional
               	RetryAfterMax *string `json:"retryAfterMax,omitempty"`
+              	//Format supports more destinations of cloudevents (which may require a specific event format)

Member

pierDipi Jul 5, 2024 •

edited

Loading

I would change the description here to be more specific

Something like

Format specifies the event format sent to subscriber and dead letter sink. 
Valid values are: 
- `json` <add link_to_cloudevents_spec_json_strucutred_format>
- `binary` <add link_to_cloudevents_spec binary format for HTTP>
- `ingress` <add description>

pkg/apis/duck/v1/delivery_types.go Outdated

               		}
               	}
+              	if ds.Format != nil {
+              		validFormats := map[string]bool{
+              			"structured": true,

Member

pierDipi Jul 5, 2024

@Cali0707 I was thinking now, since "structured" is not an event format but rather "JSON" is an event format, should we use JSON, so that if we would need to support "XML" or "protobuf" we could?

Member

Cali0707 Jul 5, 2024

That's a good point- let's use JSON instead then. @EraKin575 can you make the necessary changes?

Contributor Author

EraKin575 Jul 5, 2024

I will

pkg/apis/duck/v1/delivery_types.go Outdated

+              			"binary":     true,
+              			"ingress":    true,
+              		}
+              		if !validFormats[*ds.Format] {

Member

pierDipi Jul 5, 2024

I would do a case insensitive comparison !validFormats[strings.ToLower(*ds.Format)]

pkg/apis/duck/v1/delivery_types.go

+              			"ingress":    true,
+              		}
+              		if !validFormats[*ds.Format] {
+              			errs = errs.Also(apis.ErrInvalidValue(*ds.Format, "format"))

Member

pierDipi Jul 5, 2024 •

edited

Loading

I would add the details of the error with the list of valid values (3rd parameter to ErrInvalidValue)

apis.ErrInvalidValue(*ds.Format, "format", <detailed error>)

Contributor Author

EraKin575 Jul 5, 2024

Thanks for the feedback! I will make the changes

pierDipi reviewed

View reviewed changes

pkg/apis/duck/v1/delivery_types.go Outdated

+              			"binary":     true,
+              			"ingress":    true,
+              		}
+              		if !validFormats[*ds.Format] {

Member

pierDipi Jul 5, 2024

This panics if ds.Format is not in the map, we need to do something like

_, ok := validFormats[*ds.Format]; !ok {

}

pierDipi reviewed

View reviewed changes

Member

pierDipi left a comment •

edited

Loading

Can we add a few test cases for the validation of Format in

eventing/pkg/apis/duck/v1/delivery_types_test.go

Line 30 in 5f6713a

func TestDeliverySpecValidation(t *testing.T) {

Member

Cali0707 commented Jul 5, 2024

@EraKin575 you will also need to run ./hack/update-codegen.sh and commit those changes, due to the API type change you made


          updated generated code

55433b1

Signed-off-by: EraKin575 <tejaskumar574@gmail.com>

knative-prow bot added size/M and removed size/S labels

EraKin575 requested review from pierDipi and Cali0707

July 5, 2024 20:40


          added test

975b5ce

Signed-off-by: EraKin575 <tejaskumar574@gmail.com>

Cali0707 approved these changes

View reviewed changes

Member

Cali0707 left a comment

/lgtm
/approve
/ok-to-test

knative-prow bot added the ok-to-test label

knative-prow bot assigned Cali0707

knative-prow bot added lgtm and removed needs-ok-to-test labels

knative-prow bot commented Jul 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, EraKin575

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Cali0707]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-prow bot added the approved label

knative-prow bot merged commit 9eed163 into knative:main

7 checks passed

Cali0707 mentioned this pull request

fix: remove ingress option from delivery format #8086

Merged

Contributor Author

EraKin575 commented Jul 11, 2024

merged my first PR at knative!

EraKin575 deleted the delivery-format branch

July 11, 2024 18:18

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm ok-to-test size/M